feat: switch to include directives in pg_hba - #1765
Open
staaldraad wants to merge 7 commits into
Open
Conversation
Member
Author
|
Requires pg16+ |
staaldraad
force-pushed
the
etienne/sec-493-switch-pg_hba-to-use-include-directive
branch
from
August 19, 2025 07:54
e258813 to
6d11c7d
Compare
staaldraad
marked this pull request as ready for review
August 26, 2025 12:52
hunleyd
requested changes
Aug 27, 2025
staaldraad
force-pushed
the
etienne/sec-493-switch-pg_hba-to-use-include-directive
branch
from
August 27, 2025 14:08
7b62c4f to
641951d
Compare
Merged
hunleyd
previously requested changes
Sep 3, 2025
hunleyd
left a comment
hunleyd
left a comment
Contributor
There was a problem hiding this comment.
lgtm, minus the conflict
staaldraad
force-pushed
the
etienne/sec-493-switch-pg_hba-to-use-include-directive
branch
from
September 8, 2025 08:27
641951d to
9a840bf
Compare
staaldraad
dismissed
hunleyd’s stale review
September 8, 2025 11:11
conflict resolved and rebased to use latest admin_api and admin_mgr as introduced by #1780
staaldraad
force-pushed
the
etienne/sec-493-switch-pg_hba-to-use-include-directive
branch
from
January 28, 2026 10:55
9a840bf to
4e6292b
Compare
Contributor
WalkthroughAdds version-aware PostgreSQL HBA handling: PostgreSQL 15 uses Changes
Sequence Diagram(s)sequenceDiagram
participant Deployer
participant VersionDetector as Version Detector
participant BuildSystem as Nix/Docker
participant Ansible
participant ConfigStore as HBA Templates
participant Postgres
Deployer->>VersionDetector: Determine Postgres major version
VersionDetector->>BuildSystem: Provide version (e.g., 15 / other)
BuildSystem->>Ansible: Trigger provisioning with psql_version
alt psql_version == 15
Ansible->>ConfigStore: Select `pg_hba.conf_15.j2`
Ansible->>Postgres: Deploy `/etc/postgresql/pg_hba.conf` (from _15 template)
else psql_version != 15
Ansible->>ConfigStore: Select `pg_hba.conf.j2`, `pg_hba_public.conf.j2`, `pg_hba_users_public.conf.j2`
Ansible->>Postgres: Deploy `/etc/postgresql/pg_hba.conf` and modular public/user HBA files
end
Postgres-->>Deployer: HBA configuration applied
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nix/packages/lib.nix (1)
11-18: Fix missingversionargument in nix/checks.nix (line 102).The call to
makePostgresDevSetupis missing the requiredversionargument. Add it to the object passed at line 102:start-postgres-server-bin = pkgs-lib.makePostgresDevSetup { inherit pkgs pgroonga; name = "start-postgres-server-test"; version = ""; # or appropriate version value extraSubstitutions = {The call in nix/packages/default.nix correctly passes
version = activeVersion.
🤖 Fix all issues with AI agents
In `@ansible/tasks/setup-pgbouncer.yml`:
- Around line 46-52: The pgbouncer user creation task uses ansible.builtin.user
with name "pgbouncer" and has a typo in the shell parameter
("/usr/sbin/nolign"); update the shell value to the correct "/usr/sbin/nologin"
in that task so the pgbouncer user gets the proper non-login shell.
In `@nix/tools/run-server.sh.in`:
- Around line 216-225: The extra HBA glob currently uses pg_hba*.conf* and can
match the base pg_hba.conf and fail when there are no matches; change the logic
in run-server.sh.in: enable nullglob (shopt -s nullglob) before building
extra_hba_files, use the narrower pattern
${PG_HBA_FILE%pg_hba.conf}pg_hba_*.conf to only match include files, iterate and
copy them as before, then restore nullglob (shopt -u nullglob) if needed; ensure
the earlier cp of "${PG_HBA_FILE}" remains and that the new glob will never
duplicate that base file.
🧹 Nitpick comments (3)
ansible/files/postgresql_config/pg_hba_public.conf.j2 (1)
1-2: Non-standard IPv6 notation.The IPv6 CIDR
::0/0is functional but the standard notation is::/0. Consider using::/0for consistency with PostgreSQL documentation and broader convention.Suggested fix
host all all 0.0.0.0/0 scram-sha-256 -host all all ::0/0 scram-sha-256 +host all all ::/0 scram-sha-256ansible/files/postgresql_config/pg_hba.conf.j2 (1)
34-35: Same IPv6 notation note for replication rules.Consider
::/0instead of::0/0for consistency.Suggested fix
host replication supabase_replication_admin 0.0.0.0/0 scram-sha-256 -host replication supabase_replication_admin ::0/0 scram-sha-256 +host replication supabase_replication_admin ::/0 scram-sha-256ansible/tasks/setup-pgbouncer.yml (1)
112-119: Duplicate entry in loop.
/etc/pgbouncer-custom/ssl-config.iniappears twice in the loop (lines 113 and 117). This is harmless but redundant.Proposed fix
loop: - /etc/pgbouncer-custom/ssl-config.ini - /etc/postgresql/pg_hba.conf - /etc/postgresql/pg_hba_users_public.conf - /etc/postgresql/pg_hba_public.conf - - /etc/pgbouncer-custom/ssl-config.ini
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nix/packages/lib.nix (1)
11-18: Addversionparameter to themakePostgresDevSetupcall in nix/checks.nix.The call at nix/checks.nix line 102 is missing the required
versionparameter. This will fail Nix evaluation. The call in nix/packages/default.nix already includesversion = activeVersion;but nix/checks.nix needs the same fix.
🤖 Fix all issues with AI agents
In `@ansible/tasks/setup-pgbouncer.yml`:
- Around line 108-120: The loop in the Ansible task named "Grant pg_hba and
pgbouncer grp perm for adminapi updates" contains a duplicate entry for
"/etc/pgbouncer-custom/ssl-config.ini"; remove the repeated item from the list
in the loop so each path is unique (adjust the array under the loop that uses
loop_var "pgbouncer_group_item"), leaving only one occurrence of
"/etc/pgbouncer-custom/ssl-config.ini".
🧹 Nitpick comments (1)
nix/packages/lib.nix (1)
70-94: Normalizeversionbefore comparing to"15".If
versionincludes a patch/build suffix (e.g.,15.14.x-hba),version == "15"will miss and route PG15 to the include-based config, which the PR notes is unsupported. Consider deriving the major version first.♻️ Suggested change
- extraPaths = - if version == "15" then + extraPaths = + let + majorVersion = lib.versions.major version; + in + if majorVersion == "15" then
staaldraad
force-pushed
the
etienne/sec-493-switch-pg_hba-to-use-include-directive
branch
2 times, most recently
from
January 28, 2026 13:19
c420bb1 to
088a3d9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ansible/tasks/setup-pgbouncer.yml`:
- Around line 108-118: The task "Grant pg_hba and pgbouncer grp perm for
adminapi updates" is trying to set modes on files that don't exist on PG15
hosts; add a conditional to skip the task on PG15 by adding when: not is_psql_15
(same pattern used in the postgres setup block) so the loop over
pgbouncer_group_item (/etc/pgbouncer-custom/ssl-config.ini,
/etc/postgresql/pg_hba.conf, /etc/postgresql/pg_hba_users_public.conf,
/etc/postgresql/pg_hba_public.conf) only runs on non-PG15 hosts.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@nix/tools/run-server.sh.in`:
- Around line 192-195: The script currently prints secret key contents by
echoing and catting the KEY_FILE (KEY_FILE, DATDIR variables); remove the cat
and the "KEY_FILE contents:" echo or gate them behind a debug-only check so
secrets are never printed in normal runs (e.g. replace the two lines with a
conditional that only prints/redacts when DEBUG or VERBOSE is explicitly
enabled, otherwise print "KEY_FILE: [REDACTED]" and keep export KEY_FILE as-is).
Ensure you target the export KEY_FILE, the echo "KEY_FILE:" and cat "$KEY_FILE"
statements for the change.
staaldraad
force-pushed
the
etienne/sec-493-switch-pg_hba-to-use-include-directive
branch
from
April 7, 2026 07:48
088a3d9 to
721d8fe
Compare
PostgreSQL Extension Dependency Analysis: PR #1765
SummaryNo extensions had dependencies with MAJOR version updates. Full Analysis ResultsPostgreSQL 15 Extension DependenciesPostgreSQL 17 Extension DependenciesOrioleDB 17 Extension Dependencies |
PostgreSQL Package Dependency Analysis: PR #1765
SummaryNo packages had MAJOR version updates. Full Analysis ResultsPostgreSQL 15 Dependency ChangesExtracting PostgreSQL 15 dependencies...
Runtime Closure Size
Raw Dependency ClosurePostgreSQL 17 Dependency ChangesExtracting PostgreSQL 17 dependencies...
Runtime Closure Size
Raw Dependency Closure |
staaldraad
force-pushed
the
etienne/sec-493-switch-pg_hba-to-use-include-directive
branch
5 times, most recently
from
April 14, 2026 11:52
eb5b8de to
ef65cc5
Compare
staaldraad
force-pushed
the
etienne/sec-493-switch-pg_hba-to-use-include-directive
branch
3 times, most recently
from
April 28, 2026 16:09
6d5e246 to
96ba894
Compare
Collaborator
|
@staaldraad are we still working on this one? |
Member
Author
nope, should be ready. fixed up ansible version to rerun tests |
hunleyd
approved these changes
Jun 10, 2026
This comment has been minimized.
This comment has been minimized.
Using include directives makes changing the pg_hba.conf on the fly more flexible. Enabling / disabling ssl enforcement for example only requires creating or removing a file, leaving the pg_hba.conf untouched. Allowing for more repeatable and stable processes and no need for regex based replace or custom parsers. This will also support the just-in-time access work by allowing jit to be dynamically enabled/disabled
Dockerfile-supabase (added after this PR was opened) is now the base image for the PG15/17 build matrix, and didn't have the per-version pg_hba split applied. include_if_exists requires PG16+, so PG15 keeps the monolithic pg_hba.conf_15.j2 and drops the include fragments.
brainrake
force-pushed
the
etienne/sec-493-switch-pg_hba-to-use-include-directive
branch
from
July 30, 2026 22:56
bb5a7e1 to
921e4b4
Compare
Collaborator
|
@staaldraad Rebased onto current develop. Resolved conflicts:
|
mkindahl
reviewed
Aug 3, 2026
mkindahl
left a comment
mkindahl
left a comment
Contributor
There was a problem hiding this comment.
I would suggest a slightly different approach to make it easy for downstream projects.
Comment on lines
+28
to
+35
| include_if_exists pg_hba_users_public_ssl.conf | ||
| include_if_exists pg_hba_pam_public_ssl.conf | ||
| include_if_exists pg_hba_public_ssl.conf | ||
|
|
||
| # otherwise, non ssl enforced rules will apply | ||
| include_if_exists pg_hba_users_public.conf | ||
| include_if_exists pg_hba_pam_public.conf | ||
| include_if_exists pg_hba_public.conf |
Contributor
There was a problem hiding this comment.
Hmm... this hard-codes all configuration file names so if a downstream user (projects that use these Dockerfiles to create their own, for example Multigres), additional files cannot be added easily. There is probably also going to be problems if any files need to be added, renamed, or removed since file names are hard-coded.
A more flexible approach is to create a directory pg_hba.conf.d (following Debian convention for configuration directories) and just use an include_dir directive. In that case, files can be added and removed at will and will not generate any errors.
Member
Author
There was a problem hiding this comment.
makes sense. At the time, I was concerned in ensuring the correct order, I think I did initially misunderstand the following from the docs:
The first record with a matching connection type, client address, requested database, and user name is used to perform authentication. There is no “fall-through” or “backup”: if one record is chosen and the authentication fails, subsequent records are not considered. If no record matches, access is denied.
(originally left pg_hba_public.conf to always exist which was incorrect and allowed sslmode=disable to skip past the ssl enforced because there was a match for host ....)
I'll see about doing this with the include_dir - it needs some Supabase internals changes to understand a pg_hba.conf.d but not a big deal
This comment has been minimized.
This comment has been minimized.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Using include directives makes changing the pg_hba.conf on the fly more flexible. Enabling / disabling ssl enforcement for example only requires creating or removing a file, leaving the pg_hba.conf untouched. Allowing for more repeatable and stable processes and no need for regex based replace or custom parsers.
This will also support the just-in-time access work by allowing jit to be dynamically enabled/disabled
The required admin-api update is included as v0.88.0 added to ansible/vars.yml
Summary by CodeRabbit
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.